Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merge feast-snowflake plugin into main repo with documentation #2193

Merged
merged 21 commits into from
Jan 31, 2022
Merged

Merge feast-snowflake plugin into main repo with documentation #2193

merged 21 commits into from
Jan 31, 2022

Conversation

sfc-gh-madkins
Copy link
Collaborator

@sfc-gh-madkins sfc-gh-madkins commented Jan 5, 2022

What this PR does / why we need it:

Allows feast to use Snowflake as an Offline & Online Store

Which issue(s) this PR fixes:

Fixes # NONE

Does this PR introduce a user-facing change?:

Allows usage of Snowflake as an offline store.

@sfc-gh-madkins sfc-gh-madkins requested a review from a team as a code owner January 5, 2022 19:54
@sfc-gh-madkins sfc-gh-madkins requested review from tsotnet and removed request for a team January 5, 2022 19:54
@feast-ci-bot
Copy link
Collaborator

Hi @sfc-gh-madkins. Thanks for your PR.

I'm waiting for a feast-dev member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@sfc-gh-madkins
Copy link
Collaborator Author

/assign @tsotnet

Copy link
Collaborator

@adchia adchia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you'll also need to change

+ make a data source creator like
self.offline_store_config = RedshiftOfflineStoreConfig(
to include snowflake in the default tests

I setup secrets for this which will be in the environment variables which you can use in tests:

SNOWFLAKE_CI_USER
SNOWFLAKE_CI_PASSWORD
SNOWFLAKE_CI_DEPLOYMENT

docs/reference/data-sources/snowflake.md Outdated Show resolved Hide resolved
protos/feast/core/DataSource.proto Outdated Show resolved Hide resolved
sdk/python/feast/infra/online_stores/snowflake.py Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Jan 6, 2022

Codecov Report

Merging #2193 (26270bb) into master (2e4f3e5) will decrease coverage by 0.97%.
The diff coverage is 39.39%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2193      +/-   ##
==========================================
- Coverage   59.67%   58.70%   -0.98%     
==========================================
  Files         108      112       +4     
  Lines        9111     9570     +459     
==========================================
+ Hits         5437     5618     +181     
- Misses       3674     3952     +278     
Flag Coverage Δ
unittests 58.70% <39.39%> (-0.98%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
sdk/python/feast/cli.py 38.65% <ø> (ø)
sdk/python/feast/data_source.py 60.15% <0.00%> (-0.73%) ⬇️
sdk/python/feast/repo_config.py 77.09% <ø> (ø)
...fline_store/test_universal_historical_retrieval.py 23.35% <0.00%> (ø)
sdk/python/feast/infra/utils/snowflake_utils.py 23.36% <23.36%> (ø)
sdk/python/feast/inference.py 60.60% <25.00%> (-1.90%) ⬇️
sdk/python/feast/type_map.py 41.61% <33.33%> (-0.18%) ⬇️
sdk/python/feast/infra/offline_stores/snowflake.py 37.93% <37.93%> (ø)
...hon/feast/infra/offline_stores/snowflake_source.py 50.42% <50.42%> (ø)
.../feature_repos/universal/data_sources/snowflake.py 51.35% <51.35%> (ø)
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2e4f3e5...26270bb. Read the comment docs.

@sfc-gh-madkins
Copy link
Collaborator Author

sfc-gh-madkins commented Jan 6, 2022

@adchia Can you also add secrets for SNOWFLAKE_CI_ROLE & SNOWFLAKE_CI_WAREHOUSE and make sure everything is case sensitive -- most objects created in snowflake are UPPERCASE

The account/user should have a database: FEAST and schema: PUBLIC created

@adchia
Copy link
Collaborator

adchia commented Jan 6, 2022

/ok-to-test

@adchia
Copy link
Collaborator

adchia commented Jan 6, 2022

@sfc-gh-madkins as mentioned offline, added all the secrets. Let me know when this is ready for another pass (With the datasourcecreator logic all taken cared of)

@adchia adchia self-assigned this Jan 6, 2022
@sfc-gh-madkins sfc-gh-madkins requested a review from adchia January 7, 2022 02:53
@sfc-gh-madkins sfc-gh-madkins requested a review from adchia January 15, 2022 21:04
@adchia adchia added the kind/feature New feature or request label Jan 18, 2022
Copy link
Collaborator

@adchia adchia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

final few nits!

sdk/python/requirements/py3.7-ci-requirements.txt Outdated Show resolved Hide resolved
sdk/python/requirements/py3.8-ci-requirements.txt Outdated Show resolved Hide resolved
docs/specs/offline_store_format.md Outdated Show resolved Hide resolved
@adchia
Copy link
Collaborator

adchia commented Jan 18, 2022

also you need to sign your commits

@adchia
Copy link
Collaborator

adchia commented Jan 18, 2022

@pyalex
Copy link
Collaborator

pyalex commented Jan 21, 2022

Hi @sfc-gh-madkins, please rebase and run make lock-python-ci-dependencies to resolve these conflicts.
Please also fix the DCO. You can probably squash your commits and sign the resulting one.

Copy link
Collaborator

@adchia adchia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bunch of nits

docs/tutorials/driver-stats-using-snowflake.md Outdated Show resolved Hide resolved
sdk/python/feast/data_source.py Outdated Show resolved Hide resolved
dmille and others added 21 commits January 30, 2022 21:37
Signed-off-by: david <[email protected]>
Signed-off-by: sfc-gh-madkins <[email protected]>
… gone (#2240)

* Delete entity from redis when the last attached feature view is deleted

Signed-off-by: pyalex <[email protected]>

* Delete entity key from Redis only when all attached feature views are gone

Signed-off-by: pyalex <[email protected]>

* make lint happy

Signed-off-by: pyalex <[email protected]>

* make lint happy

Signed-off-by: pyalex <[email protected]>

* one more try with mypy

Signed-off-by: pyalex <[email protected]>
Signed-off-by: sfc-gh-madkins <[email protected]>
Signed-off-by: Michelle Rascati <[email protected]>
Signed-off-by: sfc-gh-madkins <[email protected]>
Signed-off-by: Judah Rand <[email protected]>
Signed-off-by: sfc-gh-madkins <[email protected]>
…ra (#2258)

* add snowflake environment vars to test framework

Signed-off-by: sfc-gh-madkins <[email protected]>

* add snowflake environment vars to test framework

Signed-off-by: sfc-gh-madkins <[email protected]>
* Refactor `UNIX_TIMESTAMP` conversion

Signed-off-by: Judah Rand <[email protected]>

* Return `UNIX_TIMESTAMP` types as `datetime` to user

Signed-off-by: Judah Rand <[email protected]>

* Fix linting errors

Signed-off-by: Judah Rand <[email protected]>

* Rename variable to something more sensible

Signed-off-by: Judah Rand <[email protected]>
Signed-off-by: sfc-gh-madkins <[email protected]>
* Run validation and inference on views and entities during plan

Signed-off-by: Felix Wang <[email protected]>

* Do not log objects that are unchanged

Signed-off-by: Felix Wang <[email protected]>

* Rename Fco to FeastObject

Signed-off-by: Felix Wang <[email protected]>

* Remove useless method

Signed-off-by: Felix Wang <[email protected]>

* Lint

Signed-off-by: Felix Wang <[email protected]>

* Always initialize registry during feature store initialization

Signed-off-by: Felix Wang <[email protected]>

* Fix usage test

Signed-off-by: Felix Wang <[email protected]>

* Remove print statements

Signed-off-by: Felix Wang <[email protected]>
Signed-off-by: sfc-gh-madkins <[email protected]>
Signed-off-by: Danny Chiao <[email protected]>
Signed-off-by: sfc-gh-madkins <[email protected]>
Signed-off-by: Danny Chiao <[email protected]>
Signed-off-by: sfc-gh-madkins <[email protected]>
Signed-off-by: Danny Chiao <[email protected]>
Signed-off-by: sfc-gh-madkins <[email protected]>
Signed-off-by: Danny Chiao <[email protected]>
Signed-off-by: sfc-gh-madkins <[email protected]>
Signed-off-by: Danny Chiao <[email protected]>
Signed-off-by: sfc-gh-madkins <[email protected]>
Signed-off-by: Danny Chiao <[email protected]>
Signed-off-by: sfc-gh-madkins <[email protected]>
…y with Java Feast Server (#2259)

Signed-off-by: NalinGHub <[email protected]>
Signed-off-by: sfc-gh-madkins <[email protected]>
Signed-off-by: sfc-gh-madkins <[email protected]>
Signed-off-by: sfc-gh-madkins <[email protected]>
Signed-off-by: sfc-gh-madkins <[email protected]>
Signed-off-by: sfc-gh-madkins <[email protected]>
@felixwang9817
Copy link
Collaborator

btw @adchia @sfc-gh-madkins the test currently failing is (I think) unrelated to this PR:

FAILED sdk/python/tests/integration/online_store/test_universal_online.py::test_online_store_cleanup[Provider: aws-Redshif-dynamodb]

I've seen this failure in a bunch of other PRs as well (see e.g. some of the integration tests running on master here; I'm not sure yet if Dynamo is just super flaky or if the test is broken - I just tried running this test locally and it also failed with the same error

the test was introduced by @pyalex in #2240, so maybe he might know what's going on

Copy link
Collaborator

@adchia adchia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@feast-ci-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: adchia, sfc-gh-madkins

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@feast-ci-bot feast-ci-bot merged commit f2bc411 into feast-dev:master Jan 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.